Skip to content

fix(tbtc): remove frequency gate on reservation checklist actions - #4278

Merged
piotr-roslaniec merged 12 commits into
reservations-epicfrom
m1/reservation-coordination-checklist
Sep 3, 2026
Merged

fix(tbtc): remove frequency gate on reservation checklist actions#4278
piotr-roslaniec merged 12 commits into
reservations-epicfrom
m1/reservation-coordination-checklist

Conversation

@piotr-roslaniec

@piotr-roslaniec piotr-roslaniec commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Reservation actions (ActionReservationAnchor/ActionReservationReanchor) were already appended to the coordination checklist by a separate commit that landed directly on reservations-epic (757c6d88f) while this branch was in flight -- that part of the original blocker this PR set out to fix is already resolved on the base branch. 757c6d88f gated those reservation entries to every 4th coordination window, though, contradicting this branch's original custody-critical/every-window design. A later merge-conflict resolution (b85294809) silently kept 757c6d88f's gated version without anyone re-deciding the tradeoff on its merits, and this PR's description was never updated to match. This PR is a deliberate reversal of 757c6d88f's gate, not an accidental divergence from it.

What's in this diff (5 files)

  • pkg/tbtc/coordination.go -- getActionsChecklist now checks reservation actions on every coordination window once the activation block is reached (same priority as ActionRedemption, no frequency gate), since a delayed acceptance/re-anchor risks the on-chain ReservationActionTimeout backstop firing before the wallet subsystem acts. Also fixes two comments: the reservation-gate rationale no longer describes a per-operator config flag that doesn't exist in the actual gate, and the Redemption-priority comment no longer overstates which actions are throughput-gated below it.
  • pkg/tbtc/coordination_test.go -- Test tables updated to match the unconditional gate (TestCoordinationExecutor_GetActionsChecklist_PostActivation's 8-case table and TestCoordinationExecutor_GetActionsChecklist_Reservations). assertChecklistOrdering's doc comment corrected to the real canonical priority order (Redemption < DepositSweep < MovedFundsSweep < MovingFunds < ReservationAnchor < ReservationReanchor < Heartbeat). TestCoordinationExecutor_GetActionsChecklist_Reservations simplified: its assertion filters both sides to reservation actions only before comparing, so the ActionRedemption entries in every expectedActions literal were inert -- renamed the field to expectedReservationActions and dropped them.
  • pkg/tbtc/marshaling.go -- Adds one-line godoc comments on ReservationAnchorProposal/ReservationReanchorProposal's Marshal/Unmarshal methods, matching the convention already used by the other proposal types in this file. Also removes dead code in ReservationReanchorProposal.Unmarshal: the length check earlier in the function already guarantees copy(...) always returns 20, so the == 0 disjunct in if copy(...) == 0 || hash == [20]byte{} could never be true; kept only the live [20]byte{} zero-value check with an explicit copy beforehand.
  • pkg/tbtc/marshaling_test.go -- Adds ReservationAnchorProposal/ReservationReanchorProposal cases to the existing marshaling-roundtrip test table, plus two new fuzz round-trip tests (TestFuzzCoordinationMessage_MarshalingRoundtrip_WithReservationAnchorProposal/...WithReservationReanchorProposal) following the same pattern as this file's existing fuzz tests for the other proposal types.
  • pkg/tbtc/reservation_test.go -- Adds two Unmarshal negative-test cases for invalid hash lengths (deposit funding tx hash on the anchor side, target wallet hash on the re-anchor side). Renames TestReservationProposals_UnmarshalRejectsMissingIntegers to TestReservationProposals_UnmarshalRejectsInvalidPayloads since its table now covers invalid-length cases too, not only missing integers, and adds a regression case ("re-anchor zero-value target wallet hash") pinning the all-zero-hash rejection the marshaling.go dead-code disjunct was masking.

Known accepted tradeoff (documented, not fixed here)

ProposalGenerator.Generate (pkg/tbtcpg/tbtcpg.go) returns on the first checklist action that yields a proposal, and reservation actions sit last in the checklist (after Redemption, DepositSweep, MovedFundsSweep, MovingFunds). A wallet with steady redemption/sweep traffic can still delay reservation acceptance/re-anchor even though the checklist entry itself is now unconditional. This is called out in a new comment at the reservation-gate site in coordination.go; it is bounded by the on-chain ReservationActionTimeout backstop and is not addressed by this PR.

Testing

  • gofmt -l ./pkg/tbtc/: clean.
  • go vet ./pkg/tbtc/...: clean. (Repo-wide go vet ./... surfaces one pre-existing, unrelated warning in pkg/tecdsa/signing/protocol.go -- a file this PR does not touch.)
  • go build ./...: succeeds.
  • go test ./...: 1846 tests passed across 89 packages, zero failures.
  • go test ./pkg/tbtc/ -run 'GetActionsChecklist|ReservationProposals': 43 tests passed, verifying every changed test case individually.

Not in this PR

  • The docs/spec/reservations/m1-keep-core-readiness/01-gap-analysis.md gap-analysis doc referenced by an earlier version of this description does not exist in this repo and is not added here.
  • Milestone 2 test-coverage backfill and the M3 multi-signer integration test are tracked as separate follow-up work.

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: faeb4a96-f94b-4403-902b-ad4b127b2f9a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@piotr-roslaniec
piotr-roslaniec force-pushed the m1/reservation-protobuf-marshaling branch from 5c1f8f5 to 87c6a68 Compare September 2, 2026 18:13
Base automatically changed from m1/reservation-protobuf-marshaling to reservations-epic September 3, 2026 05:49
@piotr-roslaniec piotr-roslaniec changed the title fix(tbtc): wire reservation actions into the coordination checklist fix(tbtc): remove frequency gate on reservation checklist actions Sep 3, 2026
Reservation actions (acceptance, re-anchor) are custody-critical like
Redemption and should be checked on every coordination window once
the activation block is reached, unlike the throughput-gated
DepositSweep/MovedFundsSweep/MovingFunds actions: an unredeemed
request risks user funds being stuck, not just throughput.

Rebased onto reservations-epic, which independently landed the same
checklist entry gated on the legacy windowIndex%4==0 flag (from a
parallel branch merged before this fix was proposed). Restore the
unconditional every-window append and update
TestCoordinationExecutor_GetActionsChecklist_PostActivation's table,
which had drifted from ReservationsActivationBlock after the base
branch bumped that constant ahead of chain tip.

Also carries forward, from the same rebase reconciliation:
- Marshal/Unmarshal doc comments on ReservationAnchorProposal and
  ReservationReanchorProposal (marshaling.go).
- Additional TestReservationProposals_UnmarshalRejectsInvalidPayloads
  cases covering target wallet public key hash validation
  (reservation_test.go).
@piotr-roslaniec
piotr-roslaniec force-pushed the m1/reservation-coordination-checklist branch from db0e0aa to b50918f Compare September 3, 2026 09:08
piotr-roslaniec and others added 11 commits September 3, 2026 10:57
Closes gap-analysis Major row 1 and implementation-plan.md M1 row 3.

ReservationAnchorProposal, ReservedRedemptionProposal,
ReservationReanchorProposal, and ReservationDissolutionProposal
previously used a JSON Marshal/Unmarshal placeholder, unlike every
other CoordinationProposal type in this package (Heartbeat,
DepositSweep, Redemption, MovingFunds, MovedFundsSweep), which all
marshal via pkg/tbtc/gen/pb.

Added the four missing message types to message.proto and
regenerated message.pb.go (protoc 3.21.12 installed for this).
Moved the four proposals' Marshal/Unmarshal from reservation.go's
JSON stubs into marshaling.go, matching the existing proto-based
implementations' structure and field-encoding conventions (big.Int
fees via .Bytes()/SetBytes(), fixed-size hashes/pubkey-hashes via
byte-slice copy with a length check).

Preserved the original JSON stubs' validation intent under proto3's
zero-value-is-absence semantics: a request nonce of 0, or empty
fee/reservation-key/hash bytes, are rejected the same way an
explicitly-missing JSON field was. The original '== nil' checks on
*big.Int fields don't carry over as-is - SetBytes never returns nil -
so they're now byte-length checks on the wire field instead, which is
the pattern every other proto-based proposal in this file already
uses.

Testing: extended the existing table-driven
TestCoordinationMessage_MarshalingRoundtrip with the four new types
(exact field-for-field equality through the wire, matching the
existing test's own precision, not just the fuzz-style tests already
covering every sibling type) plus four new
TestFuzzCoordinationMessage_MarshalingRoundtrip_With<X>Proposal
crash-safety tests, matching the one-per-type convention. Rewrote
the pre-existing TestReservationProposals_UnmarshalRejectsMissingIntegers
(now TestReservationProposals_UnmarshalRejectsInvalidFields) to
construct real protobuf payloads instead of JSON string literals,
porting every original missing-field case plus two new structural
cases (invalid hash/pubkey-hash length) that fall out of the new
wire format.

go test ./pkg/tbtc/...: 15/15 new/changed tests pass, full package
suite passes (146s), -race clean (156s). gofmt/vet clean on all 6
changed files.
…oordination

Implementation-plan.md Milestone 3, 'multi-signer simulated
integration test' item (per user decision: build the test, leave the
testnet-drill item as an agent-not-actionable tracked item since it
needs live infra and calendar time, not code).

Scales TestCoordinationExecutor_Coordinate's existing 3-operator
harness - deterministic keypairs, real per-operator localChain fakes,
a real shared netlocal.BroadcastChannel, one goroutine per operator
running coordinationExecutor.coordinate concurrently - to
ReservationAnchorProposal and ReservationReanchorProposal. This
exercises the real leader/follower coordination round-trip
(checklist generation -> leader election -> broadcast -> follower
validation -> convergence) that no mocked pkg/tbtcpg unit test can
cover, since those call task.Run(request) directly and never go
through coordinationExecutor.coordinate. It also exercises PR
#4277's protobuf marshaling of both proposal types over a real wire
round-trip, since every follower unmarshals the leader's broadcast
coordinationMessage.

Depends on PR #4278 (this branch's parent): before that fix,
ActionReservationAnchor/ActionReservationReanchor never appeared in
getActionsChecklist's output, so every operator's checklist search in
these tests would fall through to NoopProposal and fail - confirmed
by temporarily reverting the checklist fix and re-running (both new
tests failed with the expected NoopProposal mismatch), then restoring
it.

Found and fixed one bug in this test's own harness during
verification: both new tests initially shared one netlocal broadcast
channel name. getBroadcastChannel's registry is keyed by name and
never releases old channels, so under -race (which changed
goroutine/channel-delivery timing enough to surface it in ~every
run), the reanchor test's follower sometimes received a stale
broadcast left over from the anchor test's leader. Fixed by giving
each test its own channel name; re-verified stable across 10
repeated -race runs plus the full non-race and race suites.

Testing:
- go test ./pkg/tbtc/...: 365/365 pass.
- go test -race ./pkg/tbtc/...: clean, no data races, including
  -count=10 on just the two new tests.
- go build ./... && go test ./...: full repo, 49 packages, zero FAIL.
- gofmt -l / go vet: clean.
Resolves all 11 confirmed findings from review of the reservation
multi-signer coordination test:

- Bound runReservationCoordinationRound's report wait with a 30s
  timeout instead of an unbounded channel receive: at
  coordinationBlock=24562800, coordinate()'s only cancel path takes
  ~28 simulated days to fire, so any follower-rejects-proposal
  regression would hang the goroutine and the test forever, killing
  every other pkg/tbtc test via the package-wide go test timeout.

- Derive each test's broadcast channel name from t.Name() plus a
  per-invocation nonce instead of a hardcoded literal: the
  coordination leader intentionally keeps retransmitting for the
  active phase's duration, so a hardcoded name risks an earlier
  invocation's leader retransmitting into a later invocation's
  followers under -count=N or a future test reusing the name.

- Migrate TestCoordinationExecutor_Coordinate onto the shared
  reservation-coordination helpers instead of its own duplicated
  inline fixture/report/sort logic, and collapse
  TestCoordinationExecutor_Coordinate_ReservationAnchor/Reanchor into
  one table-driven TestCoordinationExecutor_Coordinate_ReservationProposals.

- Drop the now-unused sort in runReservationCoordinationRound (no
  assertion depended on report order) and the tautological
  reports-count assertions.

- Stop aliasing the mock generator's returned pointer as the expected
  result in assertions, so the leader-side comparison isn't a
  vacuous pointer-identity check.

- Correct four doc comments that overclaimed shared-state absence,
  stale branch provenance, and reanchor test chronology; add the
  missing public-key-hash comment in newReservationCoordinationWallet.

Verified: go build ./..., go vet ./pkg/tbtc/..., gofmt clean,
go test ./pkg/tbtc/... (145s, all pass), and the three affected
tests under -race -count=10 (clean).
Removing the tautological reports-count assertion (previous commit)
also removed the only check that all three operators actually
reported: len(reports) == len(operators) holds by loop construction
regardless of *which* operators reported, so a fan-in bug returning
two reports for one operator while another's is lost would pass
silently. Add an explicit check in runReservationCoordinationRound
(which owns the fan-in) that every operator index 1..len(operators)
appears at least once among the collected reports.

Verified: go build ./..., go vet ./pkg/tbtc/..., gofmt clean,
the three affected tests individually confirmed via raw (non-
summarized) test output, -race -count=10 clean, and the full
pkg/tbtc suite (146s, all pass).
Root-causes finding P1-#2's minimum fix (unique channel name per test
invocation, previous commit): pkg/net/local's broadcastChannels registry
is append-only and process-global, and each retransmission ticker was
started with context.Background(), so it retransmits forever with no
way to stop it externally. A later test/invocation reusing a channel
name would keep receiving an earlier invocation's stale, still-
retransmitting messages for the lifetime of the test binary - three
pre-existing tests (ExecuteLeaderRoutine, ExecuteFollowerRoutine,
ExecuteFollowerRoutine_WithIdleLeader) still hardcode "test"/"test-idle"
and were never covered by the minimum fix.

- pkg/net/local/broadcast_channel_manager.go: give each channel a
  cancellable context instead of context.Background(), track the
  cancel funcs, and add ResetForTesting() to cancel every outstanding
  ticker and clear the registry.
- pkg/tbtc/coordination_test.go: wire t.Cleanup(netlocal.ResetForTesting)
  into all four broadcast-channel-creation sites in this file (the
  shared reservation-coordination helper plus the three pre-existing
  hardcoded-name tests), so every test starts from an empty registry
  regardless of channel-name convention - removing the need for the
  per-invocation-nonce workaround to be the only safeguard.

Verified: go build ./..., go vet ./pkg/tbtc/... ./pkg/net/local/...,
gofmt clean. All 5 affected tests together under -race -count=10
(50/50 pass, proving cross-invocation isolation actually holds now).
Full pkg/net/local and pkg/tbtc suites pass (145s).
ResetForTesting (previous commit) already makes channel-name reuse
safe by cancelling every outstanding ticker and clearing the registry
between invocations - proven experimentally: forcing all operators
onto one fixed colliding name still passed 20/20 under -race -count=10
with the hook active, and failed under the same forced collision with
the hook disabled (reanchor received a stale anchor proposal from an
earlier subtest's still-retransmitting leader).

The per-invocation time.Now().UnixNano() nonce was therefore dead
weight, and the doc comment claiming a name "should be unique per
test invocation" was no longer true. Dropped the nonce (channelName
is now just t.Name(), kept for attributing a leak to its source test,
not for uniqueness) and rewrote the comment to describe the actual
current invariant.

Verified: go build ./..., go vet ./pkg/tbtc/..., gofmt clean. The five
netlocal-using tests together under -race -count=20 (100/100 pass,
genuine repeated-invocation collision on the same fixed name, not a
synthetic one). Full pkg/tbtc suite (146s) green.
…r cleanup

- Rescope ResetForTesting to a name-keyed ReleaseBroadcastChannel(name)
  instead of wiping the entire process-global channel registry, so
  tests (and any future caller) can release one channel without
  destroying every other channel's retransmission ticker.
- Guard the retransmission Ticker's post-loop handler cleanup with the
  same mutex used everywhere else in the type, closing a race between
  concurrent onTick/onUnregister callers and ticker shutdown.
- Add TestReleaseBroadcastChannel covering release-stops-retransmission
  and reuse-after-release-only-delivers-to-the-new-channel behavior.
- Fix checklist-ordering doc comment to match the actual actionPriority
  map.
- Hoist the 30s fan-in deadline outside the report-collection loop so
  it bounds the whole wait instead of re-arming on every report.
- Rewrite the protocolLatch doc comment: it does not serialize
  concurrent operator goroutines, only bounds in-flight work.
- Rename reservationCoordination* test helpers to drop the misleading
  "reservation" prefix; they exercise the general coordination path.
- Fix the leader-goroutine/waiter leak in waitForBlockHeight by
  translating the requested absolute block height into the local
  chain fake's own relative counter frame before waiting, instead of
  waiting on the raw absolute height (which could take days of
  simulated block time to reach for mainnet-scale values).
- Correct the fixture doc comment's chain-sharing overclaim.
- Fix coordination.go's redemption-priority comment to describe the
  actual post-activation gating behavior.
- Rename TestReservationProposals_UnmarshalRejectsMissingIntegers to
  TestReservationProposals_UnmarshalRejectsInvalidPayloads, matching
  what the test actually covers.
…ion doc comment

The rebase's conflict resolution left the doc comment referencing the
function's pre-export lowercase name.
…oordination (#4279)

## Summary

Implements `implementation-plan.md` Milestone 3's "multi-signer
simulated
integration test" item - the last piece of the full M1
keep-core-readiness
implementation plan (M0 is an external release-coordination gate, not a
code
task; M1's four rows and M2's test-coverage backfill are covered by
[#4276](#4276),
[#4277](#4277),
[#4278](#4278), and a
separate M2 follow-up PR).

Stacked on `m1/reservation-coordination-checklist`
([#4278](#4278)).

**Scope note (per explicit decision this session):** Milestone 3 has two
items - this test, and a "testnet round with a forced liveness/stranding
drill" (~2 weeks, needs a live testnet deployment and real
multi-operator
wall-clock timing). Only the former is code; the latter is tracked as an
agent-not-actionable item in the plan doc, unchanged by this PR.

## Change

Scales `TestCoordinationExecutor_Coordinate`'s existing 3-operator
harness -
deterministic keypairs, real per-operator `localChain` fakes, a real
shared
`netlocal.BroadcastChannel`, one goroutine per operator running
`coordinationExecutor.coordinate` concurrently - to
`ReservationAnchorProposal`
and `ReservationReanchorProposal`, added as one table-driven test with
`anchor`/`reanchor` subtests:

- `TestCoordinationExecutor_Coordinate_ReservationProposals`

This exercises the real leader/follower coordination round-trip
(checklist
generation -> leader election -> broadcast -> follower validation ->
convergence) that no mocked `pkg/tbtcpg` unit test can cover, since
those
call `task.Run(request)` directly and never go through
`coordinationExecutor.coordinate`. It also exercises #4277's protobuf
marshaling of both proposal types over a real wire round-trip, since
every
follower unmarshals the leader's broadcast `coordinationMessage`.

**Depends on #4278** (this branch's parent): before that fix,
`ActionReservationAnchor`/`ActionReservationReanchor` never appeared in
`getActionsChecklist`'s output, so every operator's checklist search in
these tests fell through to `NoopProposal` and failed. Verified
directly:
temporarily reverted #4278's checklist change, re-ran the new test (both
subtests failed with the expected `NoopProposal` mismatch), then
restored it.

## A bug found in this test's own harness, and its root-cause fix

The two reservation subtests initially shared one `netlocal` broadcast
channel name. `getBroadcastChannel`'s registry is keyed by name, is
process-global, and never released old channels' retransmission tickers
(they were wired to `context.Background()`), so under `-race` the
reanchor
subtest's follower sometimes received a stale broadcast left over from
the
anchor subtest's leader - a cross-test data race in the test harness
itself, not in the production code under test.

Root-caused and fixed in `pkg/net/local` (production, non-test code,
since
the registry it fixes is used by every test file that exercises a
simulated
local network): each broadcast channel's retransmission ticker context
is
now cancellable, and a new `ReleaseBroadcastChannel(name string)`
cancels
and de-registers a channel's own ticker(s) by name (scoped to the
caller's
own channel, not a global reset) on `t.Cleanup`. This is now wired into
all
four broadcast-channel-creation sites in `pkg/tbtc/coordination_test.go`
(the shared operator helper plus three pre-existing hardcoded-name
tests),
each releasing under its own channel name.

## Testing

- `go build ./...`, `go vet ./...`, `gofmt -l`: clean.
- `go test ./pkg/tbtc/...` and `go test -race ./pkg/tbtc/...`: full
suite
  green, including `-count=10` targeted at the new/changed coordination
  tests.
- `go test ./pkg/net/local/... ./pkg/net/retransmission/...` (incl.
`-race`):
  green, including new coverage for `ReleaseBroadcastChannel`'s actual
effect (a released channel's ticker stops retransmitting; releasing and
  reopening under the same name only delivers to the new registration).

## Not in this PR

- The testnet-round liveness/stranding drill (Milestone 3's other item)
-
  operational, not code; tracked separately.
- Milestone 2's test-coverage backfill - separate follow-up PR.
@piotr-roslaniec
piotr-roslaniec marked this pull request as ready for review September 3, 2026 13:02
@piotr-roslaniec
piotr-roslaniec merged commit 19b0a4f into reservations-epic Sep 3, 2026
17 checks passed
@piotr-roslaniec
piotr-roslaniec deleted the m1/reservation-coordination-checklist branch September 3, 2026 13:02
piotr-roslaniec added a commit that referenced this pull request Sep 3, 2026
…nt review of #4282) (#4283)

## Summary

Remediation for the 37 confirmed findings from a multi-agent review of
PR #4282
(`dev` <- `reservations-epic`, i.e. the accumulated content of
#4274+#4276+#4277).
37 raised -> 37 confirmed -> 0 dropped after arbitration and validation.

- **P1 (6 of 7 fully fixed, 1 partially fixed):** deposit-sweep
reservation-vault
exclusion, reservation look-back underflow + target-wallet check,
reservation
acceptance `eth_getLogs` bounds + nonce reconciliation + caps, SPV
proof-loop
retry-eviction data loss (symptom fixed, structural root cause deferred
- see
below), stale-deposit timeout memoization, below-dust re-anchor trigger
removal
  (M-27, resolved via tbtc-v2 source after user escalation).
- **P2/P3 (22 of 30 fixed, 8 explicitly deferred):** see "Deferred"
below.

Full-repo `go build`, `go vet`, and `go test ./...` all pass with these
fixes
applied (verified after every commit and once more at closeout).

## Deferred (1 P1 architectural root-cause + 7 P2/P3 symptoms/hygiene)

An arbiter-recommended structural fix for M-16 (remove the SPV proof
loop's
persistent-cursor design entirely in favor of the stateless
bounded-rescan
pattern every sibling proof type already uses) was attempted together
with
the M-7 nonce-aware timeout fix and an M-14 dead-code removal. That
combined
change broke three existing tests and was reverted rather than debugged
under
time pressure. Only a narrower, independently-safe subset landed: a
surgical
patch for M-3 (non-lossy cursor rewind) plus unrelated
memoization/metrics/test
fixes. **M-16's own P1 rating is only partially addressed** - the
persistent-cursor
design itself, and the M-7/M-14 symptoms it also breeds, remain
unremoved.

1. **M-16 (P1)** `pkg/maintainer/spv/reservation_proof_loop.go:227-246`
-
`reservationProofScanState`'s persistent cursor is the structural root
cause of M-3 (fixed surgically) and M-7 (below). Removing it in favor of
   the stateless bounded-rescan pattern is what broke 3 tests on first
   attempt and remains unimplemented.
2. **M-7 (P2)** `reservation_action_timeout_watch.go:260-281` -
`CheckReservationActionTimeouts` deletes `pendingActions` entries on 3
of
   4 non-notifying outcomes without asserting the tracked `requestNonce`
   against the freshly-derived one; same root cause as M-3.
3. **P2** `reservation_action_timeout_watch.go:370` +
`reservation_wiring.go:38-49` -
the timeout watcher's `WalletMembersResolver` only resolves wallets the
   local operator co-signs; an offline/disabled/colluding wallet's own
   operators get zero independent timeout coverage.
4. **P2** dead-code cluster in `reservation_proof_loop.go` /
`reservation_proof_loop_test.go` -
`findReservationAcceptanceTransaction`,
`findReservationReanchorTransaction`, and their wrapper helpers have
zero
production callers; 14 tests exercise the unused wrapper instead of the
   `isMatching*` predicates actually called in production.
5. **P2** `reservation_proof_loop.go:612,~817` - two tautological guards
are
algebraically always-false, masking that the real enforced constraint is
   only `0 < fee <= TxMaxFee`.
6. **P2** `reservation_wiring.go:237-320` `startStaleDepositPoll` - the
entire loop body runs untested inside a goroutine; existing tests assert
   only that the goroutine starts.
7. **P3** `reservation_action_timeout_watch.go:18-20` - unused
   "backward-compatibility alias" constant, zero references.
8. **P3** `reservation_proof_loop.go:644` - duplicated, truncated
comment
   fragment left by a merge.

## Known conflicts with other open PRs in this stack - read before
merging

This branched from `reservations-epic` at `bb3dcb398`. Three other
efforts are
in flight against overlapping code and were **not** reconciled here,
since they
belong to PRs this one doesn't own:

### 1. `pkg/tbtc/coordination.go` vs #4278 (hard conflict, not cosmetic)

#4278 ("remove frequency gate on reservation checklist actions") drops
`&& windowIndex%frequencyWindows == 0` from the reservation-actions
checklist
gate (custody-critical, should run every window like `ActionRedemption`)
but
its diff still references the old single `ReservationsActivationBlock`
constant. This PR's `602d0ef11` independently rewrote that same `if`
into
`reservationsActivationBlock(ce.ethereumNetwork)`, a per-network table
lookup
(`ethereum.Mainnet: 26500000`, everything else defaults to 0).

**A conflict resolution that naively favors this PR's side of that hunk
silently reinstates the frequency gate #4278 deliberately removed.**
Combined
resolution (verified against both intents):

```go
// Reservation actions (acceptance, re-anchor) are custody-critical like
// Redemption and are checked on every coordination window once the
// activation block is reached, not frequency-gated like the
// throughput-driven DepositSweep/MovedFundsSweep/MovingFunds actions
// above: a delayed reservation acceptance or re-anchor risks the
// on-chain ReservationActionTimeout backstop firing before the wallet
// subsystem gets a chance to act. The activation block is a per-network
// table (reservationsActivationBlock), not a single global constant, but
// it is still config-independent and globally observable from chain
// height alone -- which is what keeps leader and follower checklists in
// agreement without relying on local config.
if coordinationBlock >= reservationsActivationBlock(ce.ethereumNetwork) {
    actions = append(actions, ActionReservationAnchor)
    actions = append(actions, ActionReservationReanchor)
}
```

### 2. `pkg/tbtc/marshaling.go` vs #4278 (duplicate, this PR's version
wins)

#4278 independently adds the same 4 missing `Marshal`/`Unmarshal` doc
comments
this PR's `7cbb8cc2f` adds, but comment-only and with a capitalization
bug
(lowercases the exported type name, e.g. `"...converts the
reservationAnchorProposal..."`). This PR's version is a superset:
correctly
capitalized comments plus the actual nil-guard/zero-hash-rejection logic
#4278 doesn't have. On merge, take this PR's 4 lines, drop #4278's.

### 3. `pkg/tbtcpg/reservation_acceptance_test.go` vs #4280 (whole-file
conflict + one real design decision)

#4280 ("M2 test-coverage backfill") independently rewrote large parts of
the
same shared test harness this PR's `726f05ed7` touched - the same
`reservationAcceptanceLocalChain` type, constructor, and ~14 shared
methods,
plus `scenarioReservationAcceptanceChain`/`registerReservedDeposits`/
`expectedAnchorsEqual`. This is a heavy line-level conflict across the
whole
file, not just redundant test names. Specifics:

- `TestReservationAcceptanceTask_AmountCapBoundaries` (this PR, cap
boundaries
only) is a strict subset of #4280's
`TestReservationAcceptanceTask_BoundaryChecks`
  (adds `MaxReservationsPerWallet`, net-of-fee `ReservationMinAmount`,
`ActiveReservationsCount`). Left in place rather than deleted
preemptively -
#4280 is still open and two-deep-stacked (on #4278, also open) and could
  stall or be reworked; delete this PR's version only in the merge that
  actually lands #4280.
- This PR's
`TestReservationAcceptanceTask_VaultNotConfigured_ZeroAddress`
(finding: dead vault-not-configured guard) has no equivalent on #4280's
  side - a "just take #4280's file" resolution silently drops it.
- **Real design decision, not just a merge conflict:** #4280's
  `TestReservationAcceptanceTask_Stateless_PastEventsError` exercises
`PastReservationAcceptanceRequestedEvents` returning an error and
asserts
fail-closed skip-on-error. This PR's `hasPendingAction` (from
`726f05ed7`)
no longer calls `PastReservationAcceptanceRequestedEvents` at all - it
uses
a different, generation-scoped pending-action check instead. Ported onto
this PR's code as-is, that test would either pass vacuously or fail for
an
unrelated reason. **This PR intentionally left
`PastReservationAcceptanceRequestedEvents`
  on the `tbtcpg.Chain` interface (`chain.go:263`) and the test double's
`acceptanceEvents`/`acceptanceEventsErr` fields in place, undeleted,
even
though they now have zero production callers** - removing them here
would
have foreclosed reconciling #4280's test against whichever
pending-action
mechanism is ultimately kept. Whoever merges this PR and #4280 needs to
pick one mechanism and either delete the losing side's interface
method/test
  or keep both if there's a reason for two independent checks.

## Testing

- `go build ./...`, `go vet ./...`: clean.
- `go test ./...`: full repo suite, 0 failures (verified at closeout
after
  every commit landed).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant